[charts] Default bar chart x-axis scale type to band #17519
Conversation
|
Thanks for adding a type label to the PR! 👍 |
bernardobelchior
added
new feature
|
Deploy preview: https://deploy-preview-17519--material-ui-x.netlify.app/ |
CodSpeed Performance ReportMerging #17519 will not alter performanceComparing Summary
|
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
github-actions
bot
added
the
PR: out-of-date
bernardobelchior
force-pushed
the
default-bar-chart-x-axis-scale-type
branch
from
6392f2a to
c23dab3
Compare
github-actions
bot
removed
the
PR: out-of-date
bernardobelchior
requested review from
alexfauquette and
JCQuintas
and removed request for
alexfauquette
| const defaultXAxis = hasHorizontalSeries ? undefined : defaultBandXAxis; | ||
| const processedXAxis = React.useMemo( | ||
| () => (xAxis ? xAxis.map((axis) => ({ scaleType: 'band' as const, ...axis })) : defaultXAxis), | ||
| [defaultXAxis, xAxis], | ||
| ); |
There was a problem hiding this comment.
If I get it correctly, this causes issues with the following config. Even if the layout is horizontal, the xAxis will get a band scale, which is not the expected behavior
<BarChart
xAxis={[{ min: 0, max: 100 }]}
layout="horizontal"
{...}
/>There was a problem hiding this comment.
Good point. Didn't think of the horizontal case. Will fix
| const defaultXAxis = hasHorizontalSeries ? undefined : defaultBandXAxis; | ||
| const processedXAxis = React.useMemo(() => { | ||
| if (!xAxis) { | ||
| return defaultXAxis; | ||
| } | ||
|
|
||
| return hasHorizontalSeries | ||
| ? xAxis | ||
| : xAxis.map((axis) => ({ scaleType: 'band' as const, ...axis })); | ||
| }, [defaultXAxis, hasHorizontalSeries, xAxis]); | ||
|
|
||
| const defaultYAxis = hasHorizontalSeries ? defaultBandYAxis : undefined; | ||
| const processedYAxis = React.useMemo(() => { | ||
| if (!yAxis) { | ||
| return defaultYAxis; | ||
| } | ||
|
|
||
| return hasHorizontalSeries | ||
| ? yAxis.map((axis) => ({ scaleType: 'band' as const, ...axis })) | ||
| : yAxis; | ||
| }, [defaultYAxis, hasHorizontalSeries, yAxis]); |
There was a problem hiding this comment.
Should we eventually move this "chart initialization" logic to the seriesConfig so we have a unified place/way of doing this?
Not for this PR though. 😆
There was a problem hiding this comment.
I'm not sure. This logic is specific to the bar chart. How would moving this to a centralized place help? Wouldn't we need to then add information about which kind of chart we're processing series for so we can know how to create the defaults? Not sure if that would help a lot 🤔
There was a problem hiding this comment.
seriesConfig is specific to each chart. Name is a bit misleading, as it also configures loads of other things 😆
There was a problem hiding this comment.
Ah, didn't know that. This requires changing the axes, though, and seriesConfig only applies to series, right?
There was a problem hiding this comment.
The seriesConfig is an object that defines aspect of the chart that is specific to the series' type. For example the way we compute axis extremums is not the same for line series or scatter series.
I don't think we can let TS deduce by itself if the xAxis or the yAxis is a band type |
We can, but it is very complicated and our types are not setup for that. This kind of issue is the common reason some libs prefer to ditch exporting their internal TS and handcraft a typescript file instead. If you have a small lib, it is kinda ok to handle a couple of extremely complex types which have all the inferences needed. But once the lib is too big, it just becomes unwieldy. |
Fixes #13794.
This fix surfaced an issue that applies to all charts when
scaleTypeisn't defined andvalueFormatteris. In that case, a type error will show because the values will implicitly haveanyas a type. To fix that, I added backscaleType: 'band'in some examples (commit).We either have wrong types or are facing a limit in TypeScript, not sure which. However, this seems to only apply for certain TypeScript configurations (
noImplicitAny: true), so this isn't a major issue IMO and this PR is an improvement over the current state of things.